fix: Improved Portainer authentication jwt expiry handling#113
fix: Improved Portainer authentication jwt expiry handling#113MaxGojko wants to merge 1 commit intodkorecko:mainfrom
Conversation
📝 WalkthroughWalkthroughA new JWT helper class is introduced to extract expiry information from JWT tokens. The Portainer authentication service is updated to use this helper for dynamic token expiry determination instead of a hard-coded 8-hour default, with additional logging added throughout the authentication flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@PatchPanda.Web/Helpers/JwtHelper.cs`:
- Around line 7-12: GetJwtExpiry currently accepts null/empty and 2-part strings
which can treat malformed tokens as valid; add an early guard in GetJwtExpiry to
return null if the jwt parameter is null, empty, or whitespace, then require
that jwt.Split('.') yields exactly 3 parts (not >=2) before proceeding; if the
parts count is not 3, return null to avoid parsing invalid tokens and invalid
caching.
In `@PatchPanda.Web/Services/PortainerService.cs`:
- Around line 99-100: Adjust how the JWT expiry is stored to include a small
clock-skew buffer: when setting _jwtExpiry from JwtHelper.GetJwtExpiry(_jwt) (or
the fallback DateTime.UtcNow.AddHours(8)), subtract a short buffer (e.g., 1–2
minutes) so you consider tokens expired slightly before the exact exp claim to
avoid near-expiry/in-flight failures; update the assignment to compute the
expiry value, apply TimeSpan.FromMinutes(1) (or chosen buffer) subtraction, and
store that in _jwtExpiry.
| public static DateTime? GetJwtExpiry(string jwt) | ||
| { | ||
| try | ||
| { | ||
| var parts = jwt.Split('.'); | ||
| if (parts.Length < 2) return null; |
There was a problem hiding this comment.
Guard against null/whitespace and malformed JWTs to avoid invalid caching.
jwt can still be null/empty at runtime, and accepting 2-part tokens can treat malformed JWTs as valid. Add an early guard and require exactly 3 parts.
🛠️ Proposed fix
public static DateTime? GetJwtExpiry(string jwt)
{
+ if (string.IsNullOrWhiteSpace(jwt)) return null;
try
{
var parts = jwt.Split('.');
- if (parts.Length < 2) return null;
+ if (parts.Length != 3) return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static DateTime? GetJwtExpiry(string jwt) | |
| { | |
| try | |
| { | |
| var parts = jwt.Split('.'); | |
| if (parts.Length < 2) return null; | |
| public static DateTime? GetJwtExpiry(string jwt) | |
| { | |
| if (string.IsNullOrWhiteSpace(jwt)) return null; | |
| try | |
| { | |
| var parts = jwt.Split('.'); | |
| if (parts.Length != 3) return null; |
🤖 Prompt for AI Agents
In `@PatchPanda.Web/Helpers/JwtHelper.cs` around lines 7 - 12, GetJwtExpiry
currently accepts null/empty and 2-part strings which can treat malformed tokens
as valid; add an early guard in GetJwtExpiry to return null if the jwt parameter
is null, empty, or whitespace, then require that jwt.Split('.') yields exactly 3
parts (not >=2) before proceeding; if the parts count is not 3, return null to
avoid parsing invalid tokens and invalid caching.
| _jwtExpiry = JwtHelper.GetJwtExpiry(_jwt) ?? DateTime.UtcNow.AddHours(8); | ||
| _logger.LogInformation("Portainer authentication successful."); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a small clock-skew buffer when storing JWT expiry.
Using the exact exp can cause near-expiry requests to fail if clocks drift or the request is in-flight.
🧩 Suggested tweak
- _jwtExpiry = JwtHelper.GetJwtExpiry(_jwt) ?? DateTime.UtcNow.AddHours(8);
+ var exp = JwtHelper.GetJwtExpiry(_jwt);
+ _jwtExpiry = exp?.AddSeconds(-60) ?? DateTime.UtcNow.AddHours(8);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _jwtExpiry = JwtHelper.GetJwtExpiry(_jwt) ?? DateTime.UtcNow.AddHours(8); | |
| _logger.LogInformation("Portainer authentication successful."); | |
| var exp = JwtHelper.GetJwtExpiry(_jwt); | |
| _jwtExpiry = exp?.AddSeconds(-60) ?? DateTime.UtcNow.AddHours(8); | |
| _logger.LogInformation("Portainer authentication successful."); |
🤖 Prompt for AI Agents
In `@PatchPanda.Web/Services/PortainerService.cs` around lines 99 - 100, Adjust
how the JWT expiry is stored to include a small clock-skew buffer: when setting
_jwtExpiry from JwtHelper.GetJwtExpiry(_jwt) (or the fallback
DateTime.UtcNow.AddHours(8)), subtract a short buffer (e.g., 1–2 minutes) so you
consider tokens expired slightly before the exact exp claim to avoid
near-expiry/in-flight failures; update the assignment to compute the expiry
value, apply TimeSpan.FromMinutes(1) (or chosen buffer) subtraction, and store
that in _jwtExpiry.
|
@MaxGojko thanks for this one as well, I'll review once you resolve coderabbit's comments. |
2 participants
Session lifetime is configurable in Portainer under Settings → Authentication → Configuration → Session lifetime. Using a longer session lifetime does not affect the program. However, using a shorter session lifetime of less than 8 hours will result in PatchPanda assuming the jwt is valid and trying to request resources with it. This results in multiple exceptions and Portainer compose stacks not being updated.
This PR improves this behavior by retrieving the expiry from the jwt token itself instead of assuming it to be 8 hours.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.